Fix incorrect parser diagnostic message for duplicate export modifier #15879
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
|
@copilot continue |
Completed the fix for the duplicate export modifier diagnostic message in commit 4ed6304. The parser now correctly reports The fix also eliminates the secondary validation error by skipping the duplicate export modifier from being added to the modifiers list. |
|
@copilot run |
Completed in commit 61c94ea. Coverage tests and entire test suite run successfully. Snapshots updated:
All tests pass including parser_misc (119/119 negative tests), parser_test262 (99.98%), parser_babel (98.20%), and parser_typescript (99.88%). |
crates/oxc_parser/src/modifiers.rs
Outdated
| if modifiers.is_empty() { | ||
| Modifiers::empty() | ||
| } else { | ||
| Modifiers::new(Some(modifiers), flags) | ||
| } |
There was a problem hiding this comment.
@copilot this if condition is unnecessary, we check on L322 that we're at a modifier. so modifiers.is_empty() will never be true.
Revert this back to the old version
Co-authored-by: camc314 <[email protected]>
- Changed diagnostic from "export modifier must precede declare modifier" to "export modifier already seen" - Skip adding duplicate export modifier to the modifiers list to avoid secondary validation error - Add test case for duplicate export modifier - Handle empty modifiers case when all modifiers are skipped Co-authored-by: camc314 <[email protected]>
- Updated parser_misc.snap to include new test case - Updated parser_typescript.snap to reflect correct diagnostic message Co-authored-by: camc314 <[email protected]>
Remove unnecessary empty check and continue statement. The check for at_modifier() on line 322 ensures modifiers is never empty when we reach the Modifiers::new() call. Co-authored-by: camc314 <[email protected]>
Snapshots now include both diagnostic messages: 1. TS(1030): 'export' modifier already seen (correct) 2. 'export' modifier cannot be used here (secondary validation) Co-authored-by: camc314 <[email protected]>
- declare-before-export-modifier.ts: Tests `declare export function foo() {}`
- duplicate-declare-modifier.ts: Tests `declare declare function foo() {}`
Both produce correct TS(1030) diagnostics for duplicate modifiers.
Co-authored-by: camc314 <[email protected]>
camc314
force-pushed
the
copilot/fix-duplicate-export-modifier
branch
from
dfe80bc to
bfe7c73
Compare
CodSpeed Performance ReportMerging #15879 will not alter performanceComparing Summary
Footnotes
|
Fix incorrect parser diagnostic message for duplicate export modifier
crates/oxc_parser/src/modifiers.rseat_modifiers_before_declarationfunctionChanges Made
Modified the
eat_modifiers_before_declarationfunction incrates/oxc_parser/src/modifiers.rsto:exportmodifier appears after an export declaration is already startedTS(1030): 'export' modifier already seen.instead of the incorrectTS(1029): 'export' modifier must precede 'declare' modifier.Added test cases:
tasks/coverage/misc/fail/duplicate-export-modifier.ts- Testsexport export function foo() {}tasks/coverage/misc/fail/declare-before-export-modifier.ts- Testsdeclare export function foo() {}tasks/coverage/misc/fail/duplicate-declare-modifier.ts- Testsdeclare declare function foo() {}Updated snapshots:
tasks/coverage/snapshots/parser_misc.snap- Added new test case entries (121 negative tests)tasks/coverage/snapshots/parser_typescript.snap- Updated diagnostic messages for duplicate export modifiersBefore
After
Testing
Fixes #15870
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.